fix: harden audit logger and add search input validation#29
fix: harden audit logger and add search input validation#29hybridindie wants to merge 1 commit intomainfrom
Conversation
Audit logger (#9): - Raise OSError on write failure instead of silently swallowing — audit log integrity is a security requirement - Reject symlinked audit file paths to prevent log redirection attacks - Cache directory creation to avoid repeated mkdir calls - Add tests for write failure and symlink rejection Search input validation (#8): - Reject empty/whitespace-only search queries - Enforce max length on query (200 chars) and model_type (50 chars) to prevent abuse of external API requests - Add tests for all validation cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Hardens AuditLogger behavior (fail-closed on write errors, mitigate log redirection) and adds input validation to search_models to reduce abuse of external model search APIs.
Changes:
- Enforce non-empty and max-length limits for
search_modelsinputs (query,model_type). - Update audit logging to raise on write failures and add a symlink-path rejection check with cached directory creation.
- Add tests covering the new search validation and audit hardening behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/comfyui_mcp/tools/models.py |
Adds search input validation limits before issuing external API calls. |
src/comfyui_mcp/audit.py |
Changes audit logger to fail closed on write errors and attempts to block symlinked audit log paths. |
tests/test_tools_models.py |
Adds tests for empty/whitespace/oversized query and oversized model_type. |
tests/test_audit.py |
Adds tests for write failures raising and for rejecting a symlinked audit log path. |
You can also share your feedback on Copilot code review. Take the survey.
| # Reject symlinked audit file — could redirect entries to attacker-controlled path | ||
| if self._audit_file.exists() and self._audit_file.is_symlink(): | ||
| raise OSError(f"Audit log path is a symlink — refusing to write: {self._audit_file}") |
There was a problem hiding this comment.
The symlink check is currently gated by self._audit_file.exists(). A dangling symlink returns exists() == False but is_symlink() == True, so this would not be rejected and open(..., 'a') would follow the symlink target. Consider checking is_symlink() unconditionally (no exists() guard), ideally immediately before each open to reduce TOCTOU risk.
| if self._dir_created: | ||
| return | ||
| parent = self._audit_file.parent | ||
| parent.mkdir(parents=True, exist_ok=True) | ||
| # Reject symlinked audit file — could redirect entries to attacker-controlled path | ||
| if self._audit_file.exists() and self._audit_file.is_symlink(): | ||
| raise OSError(f"Audit log path is a symlink — refusing to write: {self._audit_file}") | ||
| self._dir_created = True |
There was a problem hiding this comment.
_ensure_directory() returns early once _dir_created is set, which also skips the symlink safety check on subsequent writes. That allows the audit file to be swapped to a symlink after the first successful (or failed) write without being detected. Consider caching only the directory creation work, but re-checking symlink/hardlink safety on every log() call (or using an os.open-based approach with O_NOFOLLOW on supported platforms).
| if self._dir_created: | |
| return | |
| parent = self._audit_file.parent | |
| parent.mkdir(parents=True, exist_ok=True) | |
| # Reject symlinked audit file — could redirect entries to attacker-controlled path | |
| if self._audit_file.exists() and self._audit_file.is_symlink(): | |
| raise OSError(f"Audit log path is a symlink — refusing to write: {self._audit_file}") | |
| self._dir_created = True | |
| parent = self._audit_file.parent | |
| if not self._dir_created: | |
| parent.mkdir(parents=True, exist_ok=True) | |
| self._dir_created = True | |
| # Reject symlinked audit file — could redirect entries to attacker-controlled path | |
| if self._audit_file.exists() and self._audit_file.is_symlink(): | |
| raise OSError(f"Audit log path is a symlink — refusing to write: {self._audit_file}") |
| (tmp_path / "readonly").mkdir() | ||
| (tmp_path / "readonly").chmod(0o444) | ||
| logger = AuditLogger(audit_file=log_file) | ||
| with pytest.raises(OSError): | ||
| logger.log(tool="test", action="called") | ||
| # Restore permissions for cleanup | ||
| (tmp_path / "readonly").chmod(0o755) |
There was a problem hiding this comment.
The permission restoration for the readonly directory only runs if the expected exception is raised. To avoid leaving tmp_path/readonly non-traversable (which can break pytest cleanup) when this test fails unexpectedly on some environments, wrap the chmod change in a try/finally to always restore permissions.
| (tmp_path / "readonly").mkdir() | |
| (tmp_path / "readonly").chmod(0o444) | |
| logger = AuditLogger(audit_file=log_file) | |
| with pytest.raises(OSError): | |
| logger.log(tool="test", action="called") | |
| # Restore permissions for cleanup | |
| (tmp_path / "readonly").chmod(0o755) | |
| readonly_dir = tmp_path / "readonly" | |
| readonly_dir.mkdir() | |
| try: | |
| readonly_dir.chmod(0o444) | |
| logger = AuditLogger(audit_file=log_file) | |
| with pytest.raises(OSError): | |
| logger.log(tool="test", action="called") | |
| finally: | |
| # Restore permissions for cleanup | |
| readonly_dir.chmod(0o755) |
| real_file = tmp_path / "real.log" | ||
| real_file.touch() | ||
| link = tmp_path / "audit.log" | ||
| os.symlink(real_file, link) | ||
| logger = AuditLogger(audit_file=link) | ||
| with pytest.raises(OSError, match="symlink"): | ||
| logger.log(tool="test", action="called") |
There was a problem hiding this comment.
The new symlink protection is only tested for a symlink pointing to an existing target. Consider adding coverage for a dangling symlink (target missing) and for replacing the audit log with a symlink after an initial successful write, to prevent regressions in the symlink-hardening logic.
|
Closing — superseded by #30 which includes all these changes plus the full P1 security/performance batch. |
Summary
OSErrorinstead of being silently swallowed — if audit logging breaks, tool calls fail rather than operating unaudited. Symlinked audit file paths are rejected to prevent log redirection attacks. Directory creation is cached to avoid repeatedmkdircalls.query(200 chars) andmodel_type(50 chars) to prevent abuse of external API requests to HuggingFace/CivitAI.Test plan
ruff checkcleanruff format --checkcleanmypyclean🤖 Generated with Claude Code